Admin api ninja spike #117

jtmst · 2024-12-13T18:17:39Z

Issue: #99

Ninja API Spike Findings

Overview

Created import/export endpoints for NOFOs
Created schemas using Ninja's ModelSchema for automatic model validation
Implemented mock bearer token authentication
removes JSON view endpoints and tests for import/export
adjusts admin fields to reflect
adds ENV var for api_token

Key Findings

*Model Validation
- Django model constraints are preserved (e.g., max_length=511 for html_id)
- Validation errors are properly caught and returned
- Complex model relationships (NOFO -> Sections -> Subsections) are handled correctly

Error Handling

Meaningful error messages with specific details
Proper error structure using ErrorSchema
Example error response:

{
  "message": "Validation error",
  "details": {
    "html_id": ["Ensure this value has at most 511 characters (it has 4081)."]
  }
}

Authentication
- Bearer token authentication implemented
- Routes protected behind authentication
- Currently using test implementation (accepts "secret" prefix)

Benefits

Schema Management
- ModelSchema reduces code duplication
- Automatically stays in sync with model changes
- Clear documentation via OpenAPI/Swagger UI at /api/docs
Maintainability
- Clear separation of concerns (schemas.py, api.py)
- Less overhead and simple setup
- Type hints and validation built-in

Recommendation

Overall Ninja seems pretty rad. I recommend continue and implement

Screenshots:
API Docs:

Schema:

pcraig3 · 2024-12-13T18:39:56Z

bloom_nofos/nofos/api/api.py

+class BearerAuth(HttpBearer):
+    def authenticate(self, request, token):
+        # For testing purposes accept any token that starts with "secret"
+        if token and token.startswith("secret"):


Maybe we harden up our security here by also accepting "s3cr3t"

pcraig3

Over this looks great!

I've updated #75 with what I think is our go-to initial schema, but let's chat about the next steps with regards to that.

pcraig3 · 2024-12-13T22:42:38Z

bloom_nofos/nofos/api/api.py

+class BearerAuth(HttpBearer):
+    def authenticate(self, request, token):
+        # For testing purposes accept any token that starts with "secret"
+        if token and token.startswith("secret"):
+            return token
+        return None


This makes sense. I think we should generate some value we can stick in an .env var that we both use for now.

Doesn't make sense for use to use Django auth because we are going to replace that soon.

Until we have actual consumers, we just need something unguessable.

pcraig3 · 2024-12-13T22:43:05Z

bloom_nofos/nofos/api/api.py

+
+        # Create NOFO
+        nofo = Nofo(**data)
+        nofo.group = "bloom"  # TODO: Get from auth token


Can be hardcoded for now 👍

pcraig3 · 2024-12-13T22:45:07Z

bloom_nofos/nofos/api/api.py

+    try:
+        data = payload.dict()
+        sections = data.pop("sections", [])
+
+        # Create NOFO
+        nofo = Nofo(**data)
+        nofo.group = "bloom"  # TODO: Get from auth token
+        nofo.full_clean()
+        nofo.save()
+
+        _build_nofo(nofo, sections)
+        nofo.save()


Very clean! Love to see it.

pcraig3 · 2024-12-13T22:45:53Z

bloom_nofos/nofos/api/api.py

+    except ValidationError as e:
+        return 400, {"message": "Validation error", "details": e.message_dict}
+    except Exception as e:
+        return 400, {"message": str(e)}


Have you been able to trigger these? When I added an html_id that was over 500 chars, it gave me back a 422 error, not one of these.

It would be rare but not impossible for full_clean to throw an error related to model validation. I'll make it more specific but keep in in there just in case

pcraig3 · 2024-12-13T22:48:34Z

bloom_nofos/nofos/api/api.py

+@api.get("/nofo/{nofo_id}", response={200: NofoSchema, 404: ErrorSchema})
+def export_nofo(request, nofo_id: int):
+    """Export a NOFO by ID"""
+    try:
+        nofo = Nofo.objects.get(id=nofo_id)
+        return 200, nofo
+    except Nofo.DoesNotExist:
+        return 404, {"message": "NOFO not found"}


Some considerations for this:

Non-published NOFOs are secret (which I guess is what the bearer token is for)

Archived NOFOs are supposed to have been deleted, so we probably should not return them. If this is the case, we should also 404 them, so that people can't guess when a deleted NOFO exists (🤷 although maybe who cares)

So yeah, to sum up, let's return 404 as well for archived NOFOs.

bloom_nofos/nofos/api/api.py

bloom_nofos/nofos/api/schemas.py

pcraig3

Hello!

This is all looking super good! Thanks so much for converting those tests over, this is such a neat idea that we changed all the code underneath but the tests still (mostly) work.

I've made some other comments in below, but I also noticed that the validation of the NOFO object doesn't match what's in our models.py.

After removing every field I could, it looks like this is our minimum viable NOFO we can import:

{
  "title": "",
  "filename": "",
  "short_name": "",
  "number": "",
  "opdiv": "",
  "agency": "",
  "tagline": "",
  "application_deadline": "",
  "subagency": "",
  "subagency2": "",
  "author": "",
  "subject": "",
  "keywords": "",
  "inline_css": "",
  "sections": [
  ]
}

Some of these are reasonable, like title, number, etc, but inline_css and subagency2 are examples where most NOFOs won't have those, so they don't need to be there (and they aren't required by models.py).

Also, one test we had earlier with the ImportJSON route would look for len(nofo.sections), so it wasn't enough to have a sections key, you would also need to have at least 1 section (and then ditto with subsections).

For this PR, it's already very large, and, to be honest, it does 95% of what we want. So let's say we can work out the sections/subsections stuff when we look at #75, and what fields are required.

For this one, let's just remove some of the rigidity. These are the fields that I would say we should not require, but use them if they exist in the data:

subagency2
inline_css
flename

If that's too complicated, let me know.

Overall, this is looking awesome and I am excited to see this go in!

pcraig3 · 2024-12-17T23:34:09Z

bloom_nofos/nofos/api/api.py

+)
+
+
+@api.post("/nofo/import", response={201: SuccessSchema, 400: ErrorSchema})


I should have mentioned this earlier, but let's follow REST conventions a little closer.

This means:

Let's use a plural object noun for this /api/nofos/{id} (add an "s")

POST route should use the same path, doesn't need 'import': /api/nofos

Return a 201 response 👍 (we are doing this)

Return the newly created object rather than a success message

Include a Location header with the URL of the newly created resource (Location: /api/nofo/123)

updated. changed to get and create method names to be more conventional as well

bloom_nofos/nofos/api/api.py

pcraig3 · 2024-12-17T23:35:07Z

bloom_nofos/nofos/api/api.py

+
+        # Create NOFO
+        nofo = Nofo(**data)
+        nofo.group = "bloom"


This is fine for now.

If we ever get to the point where people are importing NOFOs other than us, we will need to change this from being hardcoded.

pcraig3 · 2024-12-17T23:37:15Z

bloom_nofos/nofos/api/schemas.py

+class NofoSchema(NofoBaseSchema):
+    sections: List[SectionSchema]
+


Looks like this solves the ordering problem! How does this work (and how did you figure it out)?

Completely by accident. Glad it worked though

pcraig3 · 2024-12-17T23:38:29Z

bloom_nofos/nofos/api/schemas.py

+class SectionSchema(ModelSchema):
+    subsections: List[SubsectionSchema]
+
+    class Config:
+        model = Section
+        model_fields = ["name", "html_id", "order", "has_section_page"]


Can we do the same thing with creating a base schema for sections as well? Would be nice to see section metadata before the list of subsections.

pcraig3 · 2024-12-17T23:40:21Z

bloom_nofos/nofos/api/tests.py

+        excluded_fields = ["id", "archived", "status", "group"]
+        for field in excluded_fields:
+            import_data.pop(field, None)


I think we don't even need to do this, right? Even if these fields are included, they aren't used.

How about if we do this:

keep them for this test

assert that the values for these on the new NOFO are different than these values (assertNotEqual)

pcraig3 · 2024-12-17T23:42:26Z

bloom_nofos/nofos/api/tests.py

+        # Load fixture data for import tests
+        fixture_path = os.path.join(
+            settings.BASE_DIR, "nofos", "fixtures", "json", "cms-u2u-25-001.json"
+        )


Great!

There are a few other fixtures in there I used for making sure bad data was overwritten (hrsa- and published-), but I think we can include those checks in the import_nofo test (see comment above) and then just remove these other fixtures.

pcraig3 · 2024-12-18T00:03:58Z

Once this PR goes in, it resolves a bunch of issues:

Spike out admin API with django ninja #99 (spike worked!)
Follow up: Spike out admin API #100 (we don't need this follow-up since the spike worked)
Admin API routes #101 (shipped!)

pcraig3

This is super, I am well pleased with where we landed on this.

Amazing work!

jtmst requested a review from pcraig3 December 13, 2024 18:17

pcraig3 reviewed Dec 13, 2024

View reviewed changes

pcraig3 requested changes Dec 16, 2024

View reviewed changes

jtmst requested a review from pcraig3 December 17, 2024 20:03

pcraig3 requested changes Dec 17, 2024

View reviewed changes

jtmst requested a review from pcraig3 December 18, 2024 15:35

jtmst added 10 commits December 18, 2024 11:43

api init

4020358

schema derived from model

a956957

cleanup, export ep

63ce7ad

view json routes removed, api tests

344f07c

return order

f9be9cc

typo

abe9974

test var overide

8c45e17

PR feedback

c47b03c

migration conflict fix

61eddce

update response object

c051f3a

jtmst force-pushed the admin-api-ninja-spike branch from 868e155 to c051f3a Compare December 18, 2024 17:01

pcraig3 approved these changes Dec 18, 2024

View reviewed changes

jtmst merged commit d42711a into main Dec 18, 2024
4 checks passed

pcraig3 mentioned this pull request Dec 19, 2024

Public API schema created #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin api ninja spike #117

Admin api ninja spike #117

jtmst commented Dec 13, 2024 •

edited

Loading

pcraig3 Dec 13, 2024

pcraig3 left a comment

pcraig3 Dec 13, 2024

pcraig3 Dec 13, 2024

pcraig3 Dec 13, 2024

pcraig3 Dec 13, 2024

jtmst Dec 17, 2024

pcraig3 Dec 13, 2024

pcraig3 left a comment

pcraig3 Dec 17, 2024 •

edited

Loading

jtmst Dec 18, 2024

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 Dec 17, 2024

jtmst Dec 18, 2024

pcraig3 Dec 17, 2024

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 commented Dec 18, 2024

pcraig3 left a comment •

edited

Loading

		)


		@api.post("/nofo/import", response={201: SuccessSchema, 400: ErrorSchema})

		class NofoSchema(NofoBaseSchema):
		sections: List[SectionSchema]

Admin api ninja spike #117

Admin api ninja spike #117

Conversation

jtmst commented Dec 13, 2024 • edited Loading

Ninja API Spike Findings

Overview

Key Findings

Benefits

Recommendation

Choose a reason for hiding this comment

pcraig3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcraig3 left a comment

Choose a reason for hiding this comment

pcraig3 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcraig3 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcraig3 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

pcraig3 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

pcraig3 commented Dec 18, 2024

pcraig3 left a comment • edited Loading

Choose a reason for hiding this comment

jtmst commented Dec 13, 2024 •

edited

Loading

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 Dec 17, 2024 •

edited

Loading

pcraig3 left a comment •

edited

Loading